feat(booking-requests): add optional booking-requests module - #348

Thanks for doing the separation. I reviewed the current head 4c5a3ae and verified that the instant-booking Stripe refund path is preserved when the package is enabled, including partial/cumulative refund movements and folio recalculation. The direction of the Stripe delegation seam is good.

I do think #348 needs another architecture/packaging pass before it is mergeable:

1. The production artifact cannot run the advertised package migrations. db:migrate points to tsx src/database/migrate.ts, but the package/Docker artifact contains only dist and package.json; the migrator is not a tsup entry and the SQL files are not copied. Please build a runnable migrator, ship the SQL assets, and test the packaged artifact rather than only the source checkout.
2. The UI opt-in is not propagated into Docker builds. Dashboard and booking use VITE_HAIP_BOOKING_REQUESTS at build time, but the Dockerfile/release build does not declare or pass it. The API can therefore enable the module while both UIs compile it out. Please wire the build argument through the supported deployment path, or expose capability from the API instead of compiling it independently.
3. The two opt-ins are not composed safely. With HAIP_BOOKING_REQUESTS off, a persisted or API-written bookingMode=request still makes core BookingEngineService.book() reject instant booking while request controllers are absent. Please reject request-mode configuration when the deployment module is unavailable and make startup/runtime behavior fail safe if the two switches disagree.
4. The package boundary is currently mostly a facade. Only the schema/migrations and a small registration wrapper live under packages/booking-requests; the controllers, domain services, Stripe handler, UI, and most tests remain in apps/*, and the package receives the API's own BookingRequestModule. The stripeHandlerToken option is also unused. This does not yet provide the isolation we discussed. Please move the vertical-slice implementation behind the package entry point and leave narrow neutral hooks in core.
5. Core migration still installs request-specific storage. Core push-schema.ts and core Drizzle schemas add booking mode, request payment provenance, accepted-pricing fields, indexes, and constraints before db:migrate:booking-requests runs. Please separate request-only DDL/schema from core; keep in core only fields that are independently justified for normal HAIP.
6. The exact default-install regression gate is missing. The current default-flow test sets HAIP_BOOKING_REQUESTS=true, runs both migration sets, and loads the module. Please add a mandatory gate with the flag absent, core migrations only, normal API boot, instant booking, and partial/full Stripe refunds. That is the safety contract this split is intended to protect.

Additional hardening:

• The package migration runner executes statements individually without a per-file transaction or migration journal/checksum; a failure can leave a partially applied schema.
• New non-migration test code still contains extensive handwritten SQL despite the repo's raw-SQL rule. Drizzle should be used for fixture CRUD/assertions; any PostgreSQL-only concurrency DDL needs a narrowly documented test exception/helper.
• feat(booking-requests): add optional booking-requests module #348 should be rebased after feat(email): add bounded timeouts for outbound email providers #349–feat(payments): ignore unrelated Stripe account webhook events #353 so the independent hardening is not duplicated and the test counts are regenerated.
• Please retain/port the migration safety and remediation coverage currently present in feat(booking-engine): add request-first booking workflow #347 but absent from this package split.

I am happy to review the updated package again. The feature work is strong; the blockers are about making the isolation and deployment behavior match the agreed architecture.

Thanks @agustinjch — working through the packaging/safety pass.

Landed on 61c86b3 (plus earlier main merge 99ed4f2):

• Runnable package migrator shipped in dist + SQL assets; Docker copies booking-requests node_modules
• VITE_HAIP_BOOKING_REQUESTS wired through Dockerfile / compose / release build-args
• bookingMode=request rejected when HAIP_BOOKING_REQUESTS is off (API + settings UI)
• Package migration ledger (booking_requests_schema_migrations + checksum); per-file auto-commit because PG forbids using a new enum value in the same transaction as ADD VALUE
• Removed unused stripeHandlerToken from root module options
• Fixed mailer for feat(email): add bounded timeouts for outbound email providers #349 EmailResult.status / outcomeUnknown
• Regression installs now use run-migrations.js (post-feat(webhook): dedupe deliveries with logicalEventId #350)

Still open (as you called out): moving the full vertical slice behind packages/booking-requests, and fully removing request-only DDL from core push-schema. Those are the remaining architecture blockers before this is truly mergeable as an optional package.

Appreciate the review.

Follow-up on bb761dd (on top of 61c86b3):

• Added the mandatory flag-OFF default-install regression gate: HAIP_BOOKING_REQUESTS unset, only core migrations run, AppModule boots without the booking-requests module (no booking_requests tables, no BookingRequestService provider), bookingMode=request is rejected, and instant booking + deposit capture + partial/full Stripe refund still pass. It's a normal *.spec.ts file so it runs under pnpm test without any CI workflow changes.
• Extracted the ephemeral-database subprocess helpers shared by that spec and the existing default-flow regression spec into regression-database-utils.ts instead of duplicating them.
• Ported feat(booking-engine): add request-first booking workflow #347's booking-request-schema.spec.ts, booking-request-migration-safety.spec.ts, and booking-request-remediation.postgres.spec.ts into the package. Scoped the first two to the tables/migrations this package now owns (the push-schema duplication they used to cross-check is gone — that DDL lives only in the package's migrations now). The remediation spec now replays migration 0032's SQL directly instead of going through push-schema, since the ledger-based migrator can't be re-run against a manually-reverted schema the way push-schema's idempotent re-apply could; kept it opt-in via BOOKING_REQUEST_REMEDIATION_LIVE_PG like the original.
• README/test-stats synced (2211 tests, 263 files).

Still open, documented in packages/booking-requests/README.md: moving the vertical slice (controllers/services/UI) behind the package, and fully de-duplicating the remaining request-related columns core push-schema still carries.

@agustinjch All six architecture blockers from your review are done on head 05e6b31 — CI is green (lint/typecheck, tests, build, release smoke, one-command demo smoke).

Also landed: migration ledger + checksum, unused stripeHandlerToken removed, #349–#353 rebase, #347 migration safety/remediation specs ported into the package, README counts synced (2222 / 266).

Package boundary now: controllers/DTOs/domain services/Stripe handler/unit specs live under packages/booking-requests. BookingRequestModule.forRoot(...) is a real DynamicModule with package-local ports; apps/api only binds them via booking-requests.bootstrap.ts (useExisting) when the flag is on. apps/api/src/modules/booking-request/ keeps only e2e/auth/regression specs.

Core schema: request-only audit_logs.booking_request_id (+ index) and request-shape payment indexes/checks removed from core. Thin hooks kept on purpose: payments.booking_request_id / idempotency_key, reservations.accepted_pricing_snapshot, charges.source_key / adjusts_charge_id, and booking_engine_config.booking_mode / payment_method_collection / form_questions (fail-safe gate still runs with the flag off). Contract in packages/database/src/push-schema-kept-fields.spec.ts.

Ready for your re-review / merge whenever you are. Thanks again for the packaging pass — it made the isolation real.

Follow-up on e71537b (CI green again): extra verification caught a flag-on boot crash — AppModule was evaluating bookingRequestsModules() at import time before preloadBookingRequestsModules() ran in main.ts, so HAIP_BOOKING_REQUESTS=true could not start in dev/prod (tests passed because they preload manually; demo smoke passes because the flag defaults off). Fixed by dynamic-importing AppModule after preload. Re-verified flag-on dev/prod boot + booking-request routes, full suite, regressions, and e2e.